Refactoring doc values sparse index enabling for the `host.name` field #121751

salvatore-campagna · 2025-02-05T11:00:13Z

In this PR, we change how the doc values sparse index is enabled for the host.name keyword field.
The initial implementation of the sparse index for host.name was introduced in #120741.

Previously, the choice between using an inverted index or a doc values sparse index was determined by the index parameter. With this change, we introduce a new final index-level setting, index.mapping.use_doc_values_sparse_index:

When the setting is true, we enable the sparse index and omit the inverted index for host.name.
When the setting is false (default), we retain the inverted index instead.

Additionally, this setting is only exposed if the doc_values_sparse_index feature flag is enabled.

This change simplifies enabling the doc values sparse index and makes the selection of indexing strategies explicit at the index level. Moreover, the setting is not dynamic and is exposed only for stateful deployments.

The plan is to enable this setting in our nightly benchmarks and evaluate its impact on LogsDB indexing throughput, storage footprint and query latency. Based on benchmarking results, we will decide whether to adopt the sparse index and determine the best way to configure it.

…ting

salvatore-campagna · 2025-02-05T11:20:30Z

server/src/main/java/org/elasticsearch/index/IndexSettings.java

        Property.Final
    );

+    public static final FeatureFlag DOC_VALUES_SPARSE_INDEX = new FeatureFlag("doc_values_sparse_index");


I moved the feature flag here since we now use it to enable the setting or not.

Keep the setting here, but move the setting registration to logsdb plugin?

salvatore-campagna · 2025-02-05T11:21:20Z

server/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java

-            }
-            return indexed.isConfigured() == false
-                && hasDocValues.getValue()
+            return hasDocValues.getValue()


Here I remove the check on index value and refactored the way we check sorting on host.name.

…ting

salvatore-campagna · 2025-02-05T14:12:20Z

server/src/test/java/org/elasticsearch/index/mapper/KeywordFieldMapperTests.java

        assertTrue(mapper.fieldType().hasDocValues());
        assertFalse(mapper.fieldType().isIndexed());
-        assertFalse(mapper.fieldType().hasDocValuesSparseIndex());
+        assertTrue(mapper.fieldType().hasDocValuesSparseIndex());


This has changed because the setting overrides the index parameter.

…ting

elasticsearchmachine · 2025-02-06T13:00:45Z

Pinging @elastic/es-storage-engine (Team:StorageEngine)

salvatore-campagna · 2025-02-06T13:01:31Z

server/src/test/java/org/elasticsearch/index/mapper/KeywordFieldMapperTests.java

-    public void testFieldTypeWithSkipDocValues_LogsDbMode() throws IOException {
-        assumeTrue("Needs feature flag to be enabled", FieldMapper.DOC_VALUES_SPARSE_INDEX.isEnabled());
-
+    public void testFieldTypeWithSkipDocValues_LogsDbModeDisabledSetting() throws IOException {


Now all these tests written in such a way that they work both for snapshot and release builds.

salvatore-campagna · 2025-02-06T13:02:06Z

@elasticsearchmachine test this please.

salvatore-campagna · 2025-02-06T14:35:31Z

server/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java

                && IndexMode.LOGSDB.equals(indexMode)
                && HOST_NAME.equals(fullFieldName)
-                && (indexSortConfig != null && indexSortConfig.hasPrimarySortOnField(HOST_NAME));
+                && indexSortConfigByHostName(indexSortConfig);


Here I assume we would like to create the sparse index on host.name no matter if it is the first field or not in the index sort configuration.

martijnvg · 2025-02-06T15:00:34Z

server/src/main/java/org/elasticsearch/common/settings/IndexScopedSettings.java

 */
 public final class IndexScopedSettings extends AbstractScopedSettings {

-    public static final Set<Setting<?>> BUILT_IN_INDEX_SETTINGS = Set.of(


Maybe register the setting in logsdb plugin? Then we don't have to make this big change here?

I will need to move also tests in KeywordFieldMapperTests which are LogsDB-specific in LogsDB plugin...because getPlugins() needs the LogsDB plugin too which we can't have as a dependency in the existing KeywordFieldMapperTests. Not having the LogsDB plugin in the test results in failures because the setting is not registered.

I see, can we avoid this big diff here?

Also note that MapperTestCase does not register settings provided by other plugins (like LogsDB)...so I would need to implement the logic there to be able to register settings provided by other plugins.

BUILT_IN_INDEX_SETTINGS is used by a few tests directly...which is why I had to do it this way. Those tests run both in snapshot and release builds, which means they fail il the setting is not registered because we register the setting conditionally depending on the feature flag. So if the setting is registered only in snapshot builds...release tests will fail with something like..."setting ... has not been registered".

Just to clarify why I need the change. The original set BUILT_IN_INDEX_SETTINGS is defined using Set.of which returns an immutable set. As a result I cannot just conditionally add a new setting based on a feature flag. The add method call on an immutable set would result in throwing an UnsupportedOperationException as a result of trying to modify a collection that is immutable. So here what I do its...create a mutable set, conditionally add the setting and later on make a copy to have the static variable as an immutable set again.

Another option would be to change existing tests to conditionally use a different set based on a feature flag...I would not do that anyway.

martijnvg · 2025-02-06T15:03:14Z

server/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java

    private final int ignoreAboveDefault;
    private final IndexMode indexMode;
    private final IndexSortConfig indexSortConfig;
+    private final boolean useDocValuesSParseIndex;


Maybe rename to: useDocValuesSkipper ?

martijnvg · 2025-02-06T15:04:14Z

server/src/main/java/org/elasticsearch/index/IndexSettings.java


+    public static final FeatureFlag DOC_VALUES_SPARSE_INDEX = new FeatureFlag("doc_values_sparse_index");
+    public static final Setting<Boolean> USE_DOC_VALUES_SPARSE_INDEX = Setting.boolSetting(
+        "index.mapping.use_doc_values_sparse_index",


Let's use the lucene name. So use_doc_values_skipper?

Note that in IndexVersions I think I cannot rename public static final IndexVersion HOSTNAME_DOC_VALUES_SPARSE_INDEX = def(9_008_0_00, Version.LUCENE_10_0_0);
I will rename everything else.

martijnvg · 2025-02-06T15:05:03Z

server/src/main/java/org/elasticsearch/index/IndexSettings.java

        Property.Final
    );

+    public static final FeatureFlag DOC_VALUES_SPARSE_INDEX = new FeatureFlag("doc_values_sparse_index");


Keep the setting here, but move the setting registration to logsdb plugin?

lkts · 2025-02-06T17:52:06Z

Can we avoid hardcoding host.name and have this setting take a list of fields that use it or something? Maybe just expose it as a mapping parameter - it seems generally useful and like something that some users would find helpful.

martijnvg · 2025-02-07T06:47:31Z

Can we avoid hardcoding host.name and have this setting take a list of fields that use it or something? Maybe just expose it as a mapping parameter

The idea for now is the hardcode it. Enabling doc value skipper only makes sense for fields part of index sorting, so I don't think for now it makes sense to make it generic. Which fields / field types end up supporting this depends on research yet to be done. For tsdb maybe _tsid, @timestamp and all dimension fields, for logsdb other fields that have a relation to host.name are also good candidates. For now, we are going to evaluate to effect replacing the indexed data structures with doc value skippers for host.name and @timestamp field with logsdb.

salvatorecampagna · 2025-02-07T10:34:17Z

Can we avoid hardcoding host.name and have this setting take a list of fields that use it or something? Maybe just expose it as a mapping parameter - it seems generally useful and like something that some users would find helpful.

Also if we expose it as a mapping parameter users will be able to use

Can we avoid hardcoding host.name and have this setting take a list of fields that use it or something? Maybe just expose it as a mapping parameter - it seems generally useful and like something that some users would find helpful.

This has been discussed and we decided not to have it exposed as a mapping parameter to avoid users start defining sparse indices on fields of their choice. Also, putting the logic to expose the mapping parameter behind a feature flag:

makes this PR more complex expanding its scope
introduces a new parameter that once introduced need to be kept and supported forever (bwc after removing the feature flag)
can be done later on if use cases arise and we see benefits for other fields too

…ting

martijnvg

Minor comments, LGTM otherwise.

martijnvg · 2025-02-07T13:46:51Z

server/src/main/java/org/elasticsearch/index/IndexSortConfig.java

        }
    }

+    public boolean hasSortOnFiled(final String fieldName) {


Suggested change

public boolean hasSortOnFiled(final String fieldName) {

public boolean hasSortOnField(final String fieldName) {

martijnvg · 2025-02-07T13:50:07Z

server/src/test/java/org/elasticsearch/index/mapper/KeywordFieldMapperTests.java

+        final KeywordFieldMapper mapper = (KeywordFieldMapper) mapperService.documentMapper().mappers().getMapper("host.name");
+        assertTrue(mapper.fieldType().hasDocValues());
+        assertFalse(IndexSettings.USE_DOC_VALUES_SKIPPER.get(settings) && mapper.fieldType().isIndexed());
+        assertEquals(IndexSettings.USE_DOC_VALUES_SKIPPER.get(settings), mapper.fieldType().hasDocValuesSkipper());


Suggested change

assertEquals(IndexSettings.USE_DOC_VALUES_SKIPPER.get(settings), mapper.fieldType().hasDocValuesSkipper());

if (FieldMapper.DOC_VALUES_SPARSE_INDEX.isEnabled()) {

assertTrue(mapper.fieldType().hasDocValuesSkipper());

} else {

assertFalse(mapper.fieldType().hasDocValuesSkipper());

}

I think this is easier to read?

I just wanted to have a different name for the setting and the feature flag...also to make it easier later to remove it.

Also the index setting is actually index.mapping.use_doc_values_skipper

martijnvg · 2025-02-07T13:51:54Z

server/src/test/java/org/elasticsearch/index/mapper/KeywordFieldMapperTests.java

+        final KeywordFieldMapper mapper = (KeywordFieldMapper) mapperService.documentMapper().mappers().getMapper("host.name");
+        assertTrue(mapper.fieldType().hasDocValues());
+        assertFalse(IndexSettings.USE_DOC_VALUES_SKIPPER.get(settings) && mapper.fieldType().isIndexed());
+        assertEquals(IndexSettings.USE_DOC_VALUES_SKIPPER.get(settings), mapper.fieldType().hasDocValuesSkipper());


Suggested change

assertEquals(IndexSettings.USE_DOC_VALUES_SKIPPER.get(settings), mapper.fieldType().hasDocValuesSkipper());

if (FieldMapper.DOC_VALUES_SPARSE_INDEX.isEnabled()) {

assertTrue(mapper.fieldType().hasDocValuesSkipper());

} else {

assertFalse(mapper.fieldType().hasDocValuesSkipper());

}

and same for the other similar assertions?

…ting

@timestamp

This PR extends the work done in #121751 by enabling a sparse doc values index for the @timestamp field in LogsDB. Similar to the previous PR, the setting index.mapping.use_doc_values_skipper will override the index mapping parameter when all of the following conditions are met: * The index mode is LogsDB. * The field name is @timestamp. * Index sorting is configured on @timestamp (regardless of whether it is a primary sort field or not). * Doc values are enabled. This ensures that only one index structure is defined on the @timestamp field: * If the conditions above are met, the inverted index is replaced with a sparse doc values index. * This prevents both the inverted index and sparse doc values index from being enabled together, reducing unnecessary storage overhead. This change aligns with our goal of optimizing LogsDB for storage efficiency while possibly maintaining reasonable query latency performance. It will enable us to run benchmarks and evaluate the impact of sparse indexing on the @timestamp field as well.

This patch extends the work done in #122161 and #121751 to also use the doc values sparse index for the _tsid fields in time-series mode indices.

feature: enable doc values sparse idnex using an index setting

c4ed6c2

salvatore-campagna added >non-issue :StorageEngine/Logs You know, for Logs labels Feb 5, 2025

salvatore-campagna self-assigned this Feb 5, 2025

elasticsearchmachine added the v9.1.0 label Feb 5, 2025

salvatore-campagna changed the title ~~Refactoring Doc Values Sparse Index Enabling for host.name~~ Refactoring doc values sparse index enabling for the host.name field Feb 5, 2025

salvatore-campagna and others added 2 commits February 5, 2025 12:17

fix: refactor check on sorting and remove index param check

c8e6ce2

Merge branch 'main' into feature/hostname-doc-values-sparse-index-set…

fee1b5a

…ting

salvatore-campagna commented Feb 5, 2025

View reviewed changes

salvatore-campagna and others added 7 commits February 5, 2025 12:27

fix: relay on default setting value

7b795ba

fix: simplify sort check on host.name

115e30c

fix: rework logic to conditionally add the new setting

598776d

Merge branch 'main' into feature/hostname-doc-values-sparse-index-set…

a9b76ce

…ting

Merge branch 'main' into feature/hostname-doc-values-sparse-index-set…

efad1cd

…ting

fix: make methods static and update javadoc

85f00a4

docs: remove javadocs, method is self-explanatory

1317e2d

salvatore-campagna commented Feb 5, 2025

View reviewed changes

salvatore-campagna and others added 7 commits February 5, 2025 17:57

Merge branch 'main' into feature/hostname-doc-values-sparse-index-set…

0f760e3

…ting

Merge branch 'main' into feature/hostname-doc-values-sparse-index-set…

0a40c32

…ting

fix: default true for snapshot builds, false otherwise

2214c97

fix: gate usage of sparse index with feature flag

7afba8e

fix: udpate test according to default value

531a3e9

test: run both in snapshot and release builds

066a1d3

Merge branch 'main' into feature/hostname-doc-values-sparse-index-set…

3015eb2

…ting

salvatore-campagna marked this pull request as ready for review February 6, 2025 13:00

salvatore-campagna requested a review from a team as a code owner February 6, 2025 13:00

salvatore-campagna requested review from martijnvg and removed request for a team February 6, 2025 13:00

elasticsearchmachine added the Team:StorageEngine label Feb 6, 2025

salvatore-campagna commented Feb 6, 2025

View reviewed changes

salvatore-campagna added the test-release Trigger CI checks against release build label Feb 6, 2025

salvatore-campagna commented Feb 6, 2025

View reviewed changes

martijnvg reviewed Feb 6, 2025

View reviewed changes

not: rename doc values sparse index to doc values skipper

eda9755

fix: delete file

41c6098

salvatore-campagna requested a review from martijnvg February 7, 2025 13:29

Merge branch 'main' into feature/hostname-doc-values-sparse-index-set…

30a0c13

…ting

martijnvg approved these changes Feb 7, 2025

View reviewed changes

Merge branch 'main' into feature/hostname-doc-values-sparse-index-set…

0fc3f4b

…ting

salvatore-campagna mentioned this pull request Feb 10, 2025

Enable a sparse doc values index for @timestamp in LogsDB #122161

Merged

salvatore-campagna added 2 commits February 10, 2025 11:59

fix: typo 'Filed' instead of 'Field'

9ab1a9a

fix: replace assertEquals with if statement

8a405f2

salvatore-campagna merged commit 27adf20 into elastic:main Feb 10, 2025
18 checks passed

jordan-powers mentioned this pull request Feb 16, 2025

Doc values sparse index on _tsid fields #122699

Merged

jordan-powers added a commit that referenced this pull request Feb 19, 2025

Doc values sparse index on _tsid fields (#122699)

bf31ee6

This patch extends the work done in #122161 and #121751 to also use the doc values sparse index for the _tsid fields in time-series mode indices.

	public boolean hasSortOnFiled(final String fieldName) {
	public boolean hasSortOnField(final String fieldName) {

-        assertEquals(IndexSettings.USE_DOC_VALUES_SKIPPER.get(settings), mapper.fieldType().hasDocValuesSkipper());
+        if (FieldMapper.DOC_VALUES_SPARSE_INDEX.isEnabled()) {
+           assertTrue(mapper.fieldType().hasDocValuesSkipper());
+        } else {
+          assertFalse(mapper.fieldType().hasDocValuesSkipper());
+        }

Refactoring doc values sparse index enabling for the host.name field #121751

Refactoring doc values sparse index enabling for the host.name field #121751

Uh oh!

Conversation

salvatore-campagna commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Feb 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

salvatore-campagna commented Feb 6, 2025

Uh oh!

salvatore-campagna Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

salvatore-campagna Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

salvatorecampagna Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

salvatorecampagna Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lkts commented Feb 6, 2025

Uh oh!

martijnvg commented Feb 7, 2025

Uh oh!

salvatorecampagna commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martijnvg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Refactoring doc values sparse index enabling for the `host.name` field #121751

Refactoring doc values sparse index enabling for the `host.name` field #121751

salvatore-campagna commented Feb 5, 2025 •

edited

Loading

salvatore-campagna Feb 6, 2025 •

edited

Loading

salvatore-campagna Feb 6, 2025 •

edited

Loading

salvatorecampagna Feb 7, 2025 •

edited

Loading

salvatorecampagna Feb 7, 2025 •

edited

Loading

salvatorecampagna commented Feb 7, 2025 •

edited

Loading